When a device has been reset on dom0 side, the vpci on Xen
side won't get notification, so the cached state in vpci is
all out of date compare with the real device state.
To solve that problem, add a new hypercall to clear all vpci
device state. When the state of device is reset on dom0 side,
dom0 can call this hypercall to notify vpci.
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Reviewed-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/x86/hvm/hypercall.c | 1 +
xen/drivers/pci/physdev.c | 43 ++++++++++++++++++++++++++++++++++++
xen/drivers/vpci/vpci.c | 9 ++++++++
xen/include/public/physdev.h | 7 ++++++
xen/include/xen/pci.h | 16 ++++++++++++++
xen/include/xen/vpci.h | 6 +++++
6 files changed, 82 insertions(+)
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 7fb3136f0c7c..0fab670a4871 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -83,6 +83,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
case PHYSDEVOP_pci_mmcfg_reserved:
case PHYSDEVOP_pci_device_add:
case PHYSDEVOP_pci_device_remove:
+ case PHYSDEVOP_pci_device_state_reset:
case PHYSDEVOP_dbgp_op:
if ( !is_hardware_domain(currd) )
return -ENOSYS;
diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
index 42db3e6d133c..1cce508a73b1 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -2,11 +2,17 @@
#include <xen/guest_access.h>
#include <xen/hypercall.h>
#include <xen/init.h>
+#include <xen/vpci.h>
#ifndef COMPAT
typedef long ret_t;
#endif
+static const struct pci_device_state_reset_method
+ pci_device_state_reset_methods[] = {
+ [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
+};
+
ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
ret_t ret;
@@ -67,6 +73,43 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
break;
}
+ case PHYSDEVOP_pci_device_state_reset: {
+ struct pci_device_state_reset dev_reset;
+ struct physdev_pci_device *dev;
+ struct pci_dev *pdev;
+ pci_sbdf_t sbdf;
+
+ if ( !is_pci_passthrough_enabled() )
+ return -EOPNOTSUPP;
+
+ ret = -EFAULT;
+ if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
+ break;
+ dev = &dev_reset.dev;
+ sbdf = PCI_SBDF(dev->seg, dev->bus, dev->devfn);
+
+ ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
+ if ( ret )
+ break;
+
+ pcidevs_lock();
+ pdev = pci_get_pdev(NULL, sbdf);
+ if ( !pdev )
+ {
+ pcidevs_unlock();
+ ret = -ENODEV;
+ break;
+ }
+
+ write_lock(&pdev->domain->pci_lock);
+ pcidevs_unlock();
+ ret = pci_device_state_reset_methods[dev_reset.reset_type].reset_fn(pdev);
+ write_unlock(&pdev->domain->pci_lock);
+ if ( ret )
+ printk(XENLOG_ERR "%pp: failed to reset vPCI device state\n", &sbdf);
+ break;
+ }
+
default:
ret = -ENOSYS;
break;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 1e6aa5d799b9..ff67c2550ccb 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
return rc;
}
+
+int vpci_reset_device_state(struct pci_dev *pdev)
+{
+ ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
+
+ vpci_deassign_device(pdev);
+ return vpci_assign_device(pdev);
+}
+
#endif /* __XEN__ */
static int vpci_register_cmp(const struct vpci_register *r1,
diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
index f0c0d4727c0b..a71da5892e5f 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -296,6 +296,13 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_add_t);
*/
#define PHYSDEVOP_prepare_msix 30
#define PHYSDEVOP_release_msix 31
+/*
+ * Notify the hypervisor that a PCI device has been reset, so that any
+ * internally cached state is regenerated. Should be called after any
+ * device reset performed by the hardware domain.
+ */
+#define PHYSDEVOP_pci_device_state_reset 32
+
struct physdev_pci_device {
/* IN */
uint16_t seg;
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 63e49f0117e9..376981f9da98 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -156,6 +156,22 @@ struct pci_dev {
struct vpci *vpci;
};
+struct pci_device_state_reset_method {
+ int (*reset_fn)(struct pci_dev *pdev);
+};
+
+enum pci_device_state_reset_type {
+ DEVICE_RESET_FLR,
+ DEVICE_RESET_COLD,
+ DEVICE_RESET_WARM,
+ DEVICE_RESET_HOT,
+};
+
+struct pci_device_state_reset {
+ struct physdev_pci_device dev;
+ enum pci_device_state_reset_type reset_type;
+};
+
#define for_each_pdev(domain, pdev) \
list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index da8d0f41e6f4..b230fd374de5 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
/* Remove all handlers and free vpci related structures. */
void vpci_deassign_device(struct pci_dev *pdev);
+int __must_check vpci_reset_device_state(struct pci_dev *pdev);
/* Add/remove a register handler. */
int __must_check vpci_add_register_mask(struct vpci *vpci,
@@ -282,6 +283,11 @@ static inline int vpci_assign_device(struct pci_dev *pdev)
static inline void vpci_deassign_device(struct pci_dev *pdev) { }
+static inline int __must_check vpci_reset_device_state(struct pci_dev *pdev)
+{
+ return 0;
+}
+
static inline void vpci_dump_msi(void) { }
static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
--
2.34.1On 17.06.2024 11:00, Jiqian Chen wrote:
> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -2,11 +2,17 @@
> #include <xen/guest_access.h>
> #include <xen/hypercall.h>
> #include <xen/init.h>
> +#include <xen/vpci.h>
>
> #ifndef COMPAT
> typedef long ret_t;
> #endif
>
> +static const struct pci_device_state_reset_method
> + pci_device_state_reset_methods[] = {
> + [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
> +};
What about the other three DEVICE_RESET_*? In particular ...
> @@ -67,6 +73,43 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> break;
> }
>
> + case PHYSDEVOP_pci_device_state_reset: {
> + struct pci_device_state_reset dev_reset;
> + struct physdev_pci_device *dev;
> + struct pci_dev *pdev;
> + pci_sbdf_t sbdf;
> +
> + if ( !is_pci_passthrough_enabled() )
> + return -EOPNOTSUPP;
> +
> + ret = -EFAULT;
> + if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
> + break;
> + dev = &dev_reset.dev;
> + sbdf = PCI_SBDF(dev->seg, dev->bus, dev->devfn);
> +
> + ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
> + if ( ret )
> + break;
> +
> + pcidevs_lock();
> + pdev = pci_get_pdev(NULL, sbdf);
> + if ( !pdev )
> + {
> + pcidevs_unlock();
> + ret = -ENODEV;
> + break;
> + }
> +
> + write_lock(&pdev->domain->pci_lock);
> + pcidevs_unlock();
> + ret = pci_device_state_reset_methods[dev_reset.reset_type].reset_fn(pdev);
... you're setting this up for calling NULL. In fact there's also no bounds
check for the array index.
Also, nit (further up): Opening figure braces for a new scope go onto their
own line. Then again I notice that apparenly _all_ other instances in this
file are doing it the wrong way, too.
Finally, is the "dev" local variable really needed? It effectively hides that
PCI_SBDF() is invoked on the hypercall arguments.
> + write_unlock(&pdev->domain->pci_lock);
> + if ( ret )
> + printk(XENLOG_ERR "%pp: failed to reset vPCI device state\n", &sbdf);
Maybe downgrade to dprintk()? The caller ought to handle the error anyway.
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
>
> return rc;
> }
> +
> +int vpci_reset_device_state(struct pci_dev *pdev)
As a target of an indirect call this needs to be annotated cf_check (both
here and in the declaration, unlike __must_check, which is sufficient to
have on just the declaration).
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -156,6 +156,22 @@ struct pci_dev {
> struct vpci *vpci;
> };
>
> +struct pci_device_state_reset_method {
> + int (*reset_fn)(struct pci_dev *pdev);
> +};
> +
> +enum pci_device_state_reset_type {
> + DEVICE_RESET_FLR,
> + DEVICE_RESET_COLD,
> + DEVICE_RESET_WARM,
> + DEVICE_RESET_HOT,
> +};
> +
> +struct pci_device_state_reset {
> + struct physdev_pci_device dev;
> + enum pci_device_state_reset_type reset_type;
> +};
This is the struct to use as hypercall argument. How can it live outside of
any public header? Also, when moving it there, beware that you should not
use enum-s there. Only handles and fixed-width types are permitted.
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
>
> /* Remove all handlers and free vpci related structures. */
> void vpci_deassign_device(struct pci_dev *pdev);
> +int __must_check vpci_reset_device_state(struct pci_dev *pdev);
What's the purpose of this __must_check, when the sole caller is calling
this through a function pointer, which isn't similarly annotated?
Jan
On 2024/6/17 22:17, Jan Beulich wrote:
> On 17.06.2024 11:00, Jiqian Chen wrote:
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -2,11 +2,17 @@
>> #include <xen/guest_access.h>
>> #include <xen/hypercall.h>
>> #include <xen/init.h>
>> +#include <xen/vpci.h>
>>
>> #ifndef COMPAT
>> typedef long ret_t;
>> #endif
>>
>> +static const struct pci_device_state_reset_method
>> + pci_device_state_reset_methods[] = {
>> + [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
>> +};
>
> What about the other three DEVICE_RESET_*? In particular ...
I don't know how to implement the other three types of reset.
This is a design form so that corresponding processing functions can be added later if necessary. Do I need to set them to NULL pointers in this array?
Does this form conform to your previous suggestion of using only one hypercall to handle all types of resets?
>
>> @@ -67,6 +73,43 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> break;
>> }
>>
>> + case PHYSDEVOP_pci_device_state_reset: {
>> + struct pci_device_state_reset dev_reset;
>> + struct physdev_pci_device *dev;
>> + struct pci_dev *pdev;
>> + pci_sbdf_t sbdf;
>> +
>> + if ( !is_pci_passthrough_enabled() )
>> + return -EOPNOTSUPP;
>> +
>> + ret = -EFAULT;
>> + if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
>> + break;
>> + dev = &dev_reset.dev;
>> + sbdf = PCI_SBDF(dev->seg, dev->bus, dev->devfn);
>> +
>> + ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
>> + if ( ret )
>> + break;
>> +
>> + pcidevs_lock();
>> + pdev = pci_get_pdev(NULL, sbdf);
>> + if ( !pdev )
>> + {
>> + pcidevs_unlock();
>> + ret = -ENODEV;
>> + break;
>> + }
>> +
>> + write_lock(&pdev->domain->pci_lock);
>> + pcidevs_unlock();
>> + ret = pci_device_state_reset_methods[dev_reset.reset_type].reset_fn(pdev);
>
> ... you're setting this up for calling NULL. In fact there's also no bounds
> check for the array index.
Oh, right. I will add checks next version.
>
> Also, nit (further up): Opening figure braces for a new scope go onto their
OK, will change in next version.
> own line. Then again I notice that apparenly _all_ other instances in this
> file are doing it the wrong way, too.
Do I need to change them in this patch?
>
> Finally, is the "dev" local variable really needed? It effectively hides that
> PCI_SBDF() is invoked on the hypercall arguments.
Will remove "dev" in next version.
>
>> + write_unlock(&pdev->domain->pci_lock);
>> + if ( ret )
>> + printk(XENLOG_ERR "%pp: failed to reset vPCI device state\n", &sbdf);
>
> Maybe downgrade to dprintk()? The caller ought to handle the error anyway.
Will downgrade in next version.
>
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
>>
>> return rc;
>> }
>> +
>> +int vpci_reset_device_state(struct pci_dev *pdev)
>
> As a target of an indirect call this needs to be annotated cf_check (both
> here and in the declaration, unlike __must_check, which is sufficient to
> have on just the declaration).
OK, will add cf_check in next version.
>
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -156,6 +156,22 @@ struct pci_dev {
>> struct vpci *vpci;
>> };
>>
>> +struct pci_device_state_reset_method {
>> + int (*reset_fn)(struct pci_dev *pdev);
>> +};
>> +
>> +enum pci_device_state_reset_type {
>> + DEVICE_RESET_FLR,
>> + DEVICE_RESET_COLD,
>> + DEVICE_RESET_WARM,
>> + DEVICE_RESET_HOT,
>> +};
>> +
>> +struct pci_device_state_reset {
>> + struct physdev_pci_device dev;
>> + enum pci_device_state_reset_type reset_type;
>> +};
>
> This is the struct to use as hypercall argument. How can it live outside of
> any public header? Also, when moving it there, beware that you should not
> use enum-s there. Only handles and fixed-width types are permitted.t
Yes, I put them there before, but enum is not permitted.
Then, do you have other suggested type to use to distinguish different types of resets, because enum can't work in the public header?
>
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
>>
>> /* Remove all handlers and free vpci related structures. */
>> void vpci_deassign_device(struct pci_dev *pdev);
>> +int __must_check vpci_reset_device_state(struct pci_dev *pdev);
>
> What's the purpose of this __must_check, when the sole caller is calling
> this through a function pointer, which isn't similarly annotated?
This is what I added before introducing function pointers, but after modifying the implementation, it was not taken into account.
I will remove __must_check and change to cf_check, according to your above comment.
>
> Jan
--
Best regards,
Jiqian Chen.
On 18.06.2024 08:25, Chen, Jiqian wrote:
> On 2024/6/17 22:17, Jan Beulich wrote:
>> On 17.06.2024 11:00, Jiqian Chen wrote:
>>> --- a/xen/drivers/pci/physdev.c
>>> +++ b/xen/drivers/pci/physdev.c
>>> @@ -2,11 +2,17 @@
>>> #include <xen/guest_access.h>
>>> #include <xen/hypercall.h>
>>> #include <xen/init.h>
>>> +#include <xen/vpci.h>
>>>
>>> #ifndef COMPAT
>>> typedef long ret_t;
>>> #endif
>>>
>>> +static const struct pci_device_state_reset_method
>>> + pci_device_state_reset_methods[] = {
>>> + [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
>>> +};
>>
>> What about the other three DEVICE_RESET_*? In particular ...
> I don't know how to implement the other three types of reset.
> This is a design form so that corresponding processing functions can be added later if necessary. Do I need to set them to NULL pointers in this array?
No.
> Does this form conform to your previous suggestion of using only one hypercall to handle all types of resets?
Yes, at least in principle. Question here is: To be on the safe side,
wouldn't we better reset state for all forms of reset, leaving possible
relaxation of that for later? At which point the function wouldn't need
calling indirectly, and instead would be passed the reset type as an
argument.
>> Also, nit (further up): Opening figure braces for a new scope go onto their
> OK, will change in next version.
>> own line. Then again I notice that apparenly _all_ other instances in this
>> file are doing it the wrong way, too.
> Do I need to change them in this patch?
No.
>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
>>>
>>> return rc;
>>> }
>>> +
>>> +int vpci_reset_device_state(struct pci_dev *pdev)
>>
>> As a target of an indirect call this needs to be annotated cf_check (both
>> here and in the declaration, unlike __must_check, which is sufficient to
>> have on just the declaration).
> OK, will add cf_check in next version.
Which may not be necessary if you go the route suggested above.
>>> --- a/xen/include/xen/pci.h
>>> +++ b/xen/include/xen/pci.h
>>> @@ -156,6 +156,22 @@ struct pci_dev {
>>> struct vpci *vpci;
>>> };
>>>
>>> +struct pci_device_state_reset_method {
>>> + int (*reset_fn)(struct pci_dev *pdev);
>>> +};
>>> +
>>> +enum pci_device_state_reset_type {
>>> + DEVICE_RESET_FLR,
>>> + DEVICE_RESET_COLD,
>>> + DEVICE_RESET_WARM,
>>> + DEVICE_RESET_HOT,
>>> +};
>>> +
>>> +struct pci_device_state_reset {
>>> + struct physdev_pci_device dev;
>>> + enum pci_device_state_reset_type reset_type;
>>> +};
>>
>> This is the struct to use as hypercall argument. How can it live outside of
>> any public header? Also, when moving it there, beware that you should not
>> use enum-s there. Only handles and fixed-width types are permitted.t
> Yes, I put them there before, but enum is not permitted.
> Then, do you have other suggested type to use to distinguish different types of resets, because enum can't work in the public header?
Do like we do everywhere else: Use a set of #define-s.
>>> --- a/xen/include/xen/vpci.h
>>> +++ b/xen/include/xen/vpci.h
>>> @@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
>>>
>>> /* Remove all handlers and free vpci related structures. */
>>> void vpci_deassign_device(struct pci_dev *pdev);
>>> +int __must_check vpci_reset_device_state(struct pci_dev *pdev);
>>
>> What's the purpose of this __must_check, when the sole caller is calling
>> this through a function pointer, which isn't similarly annotated?
> This is what I added before introducing function pointers, but after modifying the implementation, it was not taken into account.
> I will remove __must_check
Why remove? Is it relevant for the return value to be checked? Or if it
isn't, why would there be a return value?
Jan
> and change to cf_check, according to your above comment.
On 2024/6/18 16:33, Jan Beulich wrote:
> On 18.06.2024 08:25, Chen, Jiqian wrote:
>> On 2024/6/17 22:17, Jan Beulich wrote:
>>> On 17.06.2024 11:00, Jiqian Chen wrote:
>>>> --- a/xen/drivers/pci/physdev.c
>>>> +++ b/xen/drivers/pci/physdev.c
>>>> @@ -2,11 +2,17 @@
>>>> #include <xen/guest_access.h>
>>>> #include <xen/hypercall.h>
>>>> #include <xen/init.h>
>>>> +#include <xen/vpci.h>
>>>>
>>>> #ifndef COMPAT
>>>> typedef long ret_t;
>>>> #endif
>>>>
>>>> +static const struct pci_device_state_reset_method
>>>> + pci_device_state_reset_methods[] = {
>>>> + [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
>>>> +};
>>>
>>> What about the other three DEVICE_RESET_*? In particular ...
>> I don't know how to implement the other three types of reset.
>> This is a design form so that corresponding processing functions can be added later if necessary. Do I need to set them to NULL pointers in this array?
>
> No.
>
>> Does this form conform to your previous suggestion of using only one hypercall to handle all types of resets?
>
> Yes, at least in principle. Question here is: To be on the safe side,
> wouldn't we better reset state for all forms of reset, leaving possible
> relaxation of that for later? At which point the function wouldn't need
> calling indirectly, and instead would be passed the reset type as an
> argument.
If I understood correctly, next version should be?
Use macros to represent different reset types.
Add switch cases in PHYSDEVOP_pci_device_state_reset to handle different reset functions.
Add reset_type as a function parameter to vpci_reset_device_state for possible future use.
+ case PHYSDEVOP_pci_device_state_reset:
+ {
+ struct pci_device_state_reset dev_reset;
+ struct pci_dev *pdev;
+ pci_sbdf_t sbdf;
+
+ if ( !is_pci_passthrough_enabled() )
+ return -EOPNOTSUPP;
+
+ ret = -EFAULT;
+ if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
+ break;
+
+ sbdf = PCI_SBDF(dev_reset.dev.seg,
+ dev_reset.dev.bus,
+ dev_reset.dev.evfn);
+
+ ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
+ if ( ret )
+ break;
+
+ pcidevs_lock();
+ pdev = pci_get_pdev(NULL, sbdf);
+ if ( !pdev )
+ {
+ pcidevs_unlock();
+ ret = -ENODEV;
+ break;
+ }
+
+ write_lock(&pdev->domain->pci_lock);
+ pcidevs_unlock();
+ /* Implement FLR, other reset types may be implemented in future */
+ switch ( dev_reset.reset_type )
+ {
+ case PCI_DEVICE_STATE_RESET_COLD:
+ case PCI_DEVICE_STATE_RESET_WARM:
+ case PCI_DEVICE_STATE_RESET_HOT:
+ case PCI_DEVICE_STATE_RESET_FLR:
+ ret = vpci_reset_device_state(pdev, dev_reset.reset_type);
+ break;
+ }
+ write_unlock(&pdev->domain->pci_lock);
+
+ if ( ret )
+ dprintk(XENLOG_ERR,
+ "%pp: failed to reset vPCI device state\n", &sbdf);
+ break;
+ }
>
>>> Also, nit (further up): Opening figure braces for a new scope go onto their
>> OK, will change in next version.
>>> own line. Then again I notice that apparenly _all_ other instances in this
>>> file are doing it the wrong way, too.
>> Do I need to change them in this patch?
>
> No.
>
>>>> --- a/xen/drivers/vpci/vpci.c
>>>> +++ b/xen/drivers/vpci/vpci.c
>>>> @@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
>>>>
>>>> return rc;
>>>> }
>>>> +
>>>> +int vpci_reset_device_state(struct pci_dev *pdev)
>>>
>>> As a target of an indirect call this needs to be annotated cf_check (both
>>> here and in the declaration, unlike __must_check, which is sufficient to
>>> have on just the declaration).
>> OK, will add cf_check in next version.
>
> Which may not be necessary if you go the route suggested above.
>
>>>> --- a/xen/include/xen/pci.h
>>>> +++ b/xen/include/xen/pci.h
>>>> @@ -156,6 +156,22 @@ struct pci_dev {
>>>> struct vpci *vpci;
>>>> };
>>>>
>>>> +struct pci_device_state_reset_method {
>>>> + int (*reset_fn)(struct pci_dev *pdev);
>>>> +};
>>>> +
>>>> +enum pci_device_state_reset_type {
>>>> + DEVICE_RESET_FLR,
>>>> + DEVICE_RESET_COLD,
>>>> + DEVICE_RESET_WARM,
>>>> + DEVICE_RESET_HOT,
>>>> +};
>>>> +
>>>> +struct pci_device_state_reset {
>>>> + struct physdev_pci_device dev;
>>>> + enum pci_device_state_reset_type reset_type;
>>>> +};
>>>
>>> This is the struct to use as hypercall argument. How can it live outside of
>>> any public header? Also, when moving it there, beware that you should not
>>> use enum-s there. Only handles and fixed-width types are permitted.t
>> Yes, I put them there before, but enum is not permitted.
>> Then, do you have other suggested type to use to distinguish different types of resets, because enum can't work in the public header?
>
> Do like we do everywhere else: Use a set of #define-s.
>
>>>> --- a/xen/include/xen/vpci.h
>>>> +++ b/xen/include/xen/vpci.h
>>>> @@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
>>>>
>>>> /* Remove all handlers and free vpci related structures. */
>>>> void vpci_deassign_device(struct pci_dev *pdev);
>>>> +int __must_check vpci_reset_device_state(struct pci_dev *pdev);
>>>
>>> What's the purpose of this __must_check, when the sole caller is calling
>>> this through a function pointer, which isn't similarly annotated?
>> This is what I added before introducing function pointers, but after modifying the implementation, it was not taken into account.
>> I will remove __must_check
>
> Why remove? Is it relevant for the return value to be checked? Or if it
> isn't, why would there be a return value?
>
> Jan
>
>> and change to cf_check, according to your above comment.
>
--
Best regards,
Jiqian Chen.
On 19.06.2024 05:39, Chen, Jiqian wrote:
> On 2024/6/18 16:33, Jan Beulich wrote:
>> On 18.06.2024 08:25, Chen, Jiqian wrote:
>>> On 2024/6/17 22:17, Jan Beulich wrote:
>>>> On 17.06.2024 11:00, Jiqian Chen wrote:
>>>>> --- a/xen/drivers/pci/physdev.c
>>>>> +++ b/xen/drivers/pci/physdev.c
>>>>> @@ -2,11 +2,17 @@
>>>>> #include <xen/guest_access.h>
>>>>> #include <xen/hypercall.h>
>>>>> #include <xen/init.h>
>>>>> +#include <xen/vpci.h>
>>>>>
>>>>> #ifndef COMPAT
>>>>> typedef long ret_t;
>>>>> #endif
>>>>>
>>>>> +static const struct pci_device_state_reset_method
>>>>> + pci_device_state_reset_methods[] = {
>>>>> + [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
>>>>> +};
>>>>
>>>> What about the other three DEVICE_RESET_*? In particular ...
>>> I don't know how to implement the other three types of reset.
>>> This is a design form so that corresponding processing functions can be added later if necessary. Do I need to set them to NULL pointers in this array?
>>
>> No.
>>
>>> Does this form conform to your previous suggestion of using only one hypercall to handle all types of resets?
>>
>> Yes, at least in principle. Question here is: To be on the safe side,
>> wouldn't we better reset state for all forms of reset, leaving possible
>> relaxation of that for later? At which point the function wouldn't need
>> calling indirectly, and instead would be passed the reset type as an
>> argument.
> If I understood correctly, next version should be?
> Use macros to represent different reset types.
> Add switch cases in PHYSDEVOP_pci_device_state_reset to handle different reset functions.
> Add reset_type as a function parameter to vpci_reset_device_state for possible future use.
>
> + case PHYSDEVOP_pci_device_state_reset:
> + {
> + struct pci_device_state_reset dev_reset;
> + struct pci_dev *pdev;
> + pci_sbdf_t sbdf;
> +
> + if ( !is_pci_passthrough_enabled() )
> + return -EOPNOTSUPP;
> +
> + ret = -EFAULT;
> + if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
> + break;
> +
> + sbdf = PCI_SBDF(dev_reset.dev.seg,
> + dev_reset.dev.bus,
> + dev_reset.dev.evfn);
> +
> + ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
> + if ( ret )
> + break;
> +
> + pcidevs_lock();
> + pdev = pci_get_pdev(NULL, sbdf);
> + if ( !pdev )
> + {
> + pcidevs_unlock();
> + ret = -ENODEV;
> + break;
> + }
> +
> + write_lock(&pdev->domain->pci_lock);
> + pcidevs_unlock();
> + /* Implement FLR, other reset types may be implemented in future */
> + switch ( dev_reset.reset_type )
> + {
> + case PCI_DEVICE_STATE_RESET_COLD:
> + case PCI_DEVICE_STATE_RESET_WARM:
> + case PCI_DEVICE_STATE_RESET_HOT:
> + case PCI_DEVICE_STATE_RESET_FLR:
> + ret = vpci_reset_device_state(pdev, dev_reset.reset_type);
> + break;
> + }
If you use a switch() here, then there wants to be a default case returning
e.g. -EOPNOTSUPP or -EINVAL. Else the switch wants dropping. I'm not sure
which one's better in this specific case, I'm only slightly tending towards
the former.
In any event the comment (if any) wants to reflect what the actual code does.
Jan
© 2016 - 2024 Red Hat, Inc.