Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-08-02 Thread Roger Pau Monné 
On Fri, Aug 02, 2024 at 08:25:58AM +0200, Jan Beulich wrote:
> On 02.08.2024 04:55, Chen, Jiqian wrote:
> > On 2024/7/31 23:55, Roger Pau Monné wrote:
> >> On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
> >>> When a device has been reset on dom0 side, the Xen hypervisor
> >>> doesn'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 support the reset
> >>> of pcidev and clear the vpci state of device. So that once the
> >>> state of device is reset on dom0 side, dom0 can call this
> >>> hypercall to notify hypervisor.
> >>>
> >>> Signed-off-by: Jiqian Chen 
> >>> Signed-off-by: Huang Rui 
> >>> Signed-off-by: Jiqian Chen 
> >>> Reviewed-by: Stewart Hildebrand 
> >>> Reviewed-by: Stefano Stabellini 
> >>
> >> Thanks, just a couple of nits.
> >>
> >> This is missing a changelog between versions, and I haven't been
> >> following all the versions, so some of my questions might have been
> >> answered in previous revisions.
> > Sorry, I will add changelogs here in next version.
> > 
> >>
> >>> ---
> >>>  xen/arch/x86/hvm/hypercall.c |  1 +
> >>>  xen/drivers/pci/physdev.c| 52 
> >>>  xen/drivers/vpci/vpci.c  | 10 +++
> >>>  xen/include/public/physdev.h | 16 +++
> >>>  xen/include/xen/vpci.h   |  8 ++
> >>>  5 files changed, 87 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..c0f47945d955 100644
> >>> --- a/xen/drivers/pci/physdev.c
> >>> +++ b/xen/drivers/pci/physdev.c
> >>> @@ -2,6 +2,7 @@
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> +#include 
> >>>  
> >>>  #ifndef COMPAT
> >>>  typedef long ret_t;
> >>> @@ -67,6 +68,57 @@ 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 pci_dev *pdev;
> >>> +pci_sbdf_t sbdf;
> >>> +
> >>> +ret = -EOPNOTSUPP;
> >>> +if ( !is_pci_passthrough_enabled() )
> >>> +break;
> >>> +
> >>> +ret = -EFAULT;
> >>> +if ( copy_from_guest(_reset, arg, 1) != 0 )
> >>> +break;
> >>> +
> >>> +sbdf = PCI_SBDF(dev_reset.dev.seg,
> >>> +dev_reset.dev.bus,
> >>> +dev_reset.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(>domain->pci_lock);
> >>> +pcidevs_unlock();
> >>> +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;
> >>> +
> >>> +default:
> >>> +ret = -EOPNOTSUPP;
> >>> +break;
> >>> +}
> >>> +write_unlock(>domain->pci_lock);
> >>> +
> >>> +break;
> >>> +}
> >>> +
> >>>  default:
> >>>  ret = -ENOSYS;
> >>>  break;
> >>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> >>> index 1e6aa5d799b9..7e914d1eff9f 100644
> >>> --- a/xen/drivers/vpci/vpci.c
> >>> +++ b/xen/drivers/vpci/vpci.c
> >>> @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
> >>>  
> >>>  return rc;
> >>>  }
> >>> +
> >>> +int vpci_reset_device_state(struct pci_dev *pdev,
> >>> +uint32_t reset_type)
> >>
> >> There's probably no use in passing reset_type to
> >> vpci_reset_device_state() if it's ignored?
> >>
> >>> +{
> >>> +ASSERT(rw_is_write_locked(>domain->pci_lock));
> >>> +
> >>> +vpci_deassign_device(pdev);
> >>> +return vpci_assign_device(pdev);
> >>> +}
> >>> +
> >>>  #endif /* __XEN__ */
> >>>  
> >>>  static int

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-08-02 Thread Jan Beulich 
On 02.08.2024 09:41, Chen, Jiqian wrote:
> On 2024/8/2 14:25, Jan Beulich wrote:
>> On 02.08.2024 04:55, Chen, Jiqian wrote:
>>> On 2024/7/31 23:55, Roger Pau Monné wrote:
 On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
> @@ -305,6 +312,15 @@ struct physdev_pci_device {
>  typedef struct physdev_pci_device physdev_pci_device_t;
>  DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t);
>  
> +struct pci_device_state_reset {
> +physdev_pci_device_t dev;
> +#define PCI_DEVICE_STATE_RESET_COLD 0
> +#define PCI_DEVICE_STATE_RESET_WARM 1
> +#define PCI_DEVICE_STATE_RESET_HOT  2
> +#define PCI_DEVICE_STATE_RESET_FLR  3
> +uint32_t reset_type;

 This might want to be a flags field, with the low 2 bits (or maybe 3
 bits to cope if more rest modes are added in the future) being used to
 signal the reset type.  We can always do that later if flags need to
 be added.
>>> Do you mean this?
>>> +struct pci_device_state_reset {
>>> +physdev_pci_device_t dev;
>>> +#define _PCI_DEVICE_STATE_RESET_COLD 0
>>> +#define PCI_DEVICE_STATE_RESET_COLD  (1U<<_PCI_DEVICE_STATE_RESET_COLD)
>>> +#define _PCI_DEVICE_STATE_RESET_WARM 1
>>> +#define PCI_DEVICE_STATE_RESET_WARM  (1U<<_PCI_DEVICE_STATE_RESET_WARM)
>>> +#define _PCI_DEVICE_STATE_RESET_HOT  2
>>> +#define PCI_DEVICE_STATE_RESET_HOT   (1U<<_PCI_DEVICE_STATE_RESET_HOT)
>>> +#define _PCI_DEVICE_STATE_RESET_FLR  3
>>> +#define PCI_DEVICE_STATE_RESET_FLR   (1U<<_PCI_DEVICE_STATE_RESET_FLR)
>>> +uint32_t reset_type;
>>> +};
>>
>> That's four bits, not two. I'm pretty sure Roger meant to keep the enum-
>> like #define-s, but additionally define a 2-bit mask constant (0x3). I
>> don't think it needs to be three bits right away - we can decide what to
>> do there when any of the higher bits are to be assigned a meaning.
> Like this?
> struct pci_device_state_reset {
> physdev_pci_device_t dev;
> #define PCI_DEVICE_STATE_RESET_COLD 0x0
> #define PCI_DEVICE_STATE_RESET_WARM 0x1
> #define PCI_DEVICE_STATE_RESET_HOT  0x2
> #define PCI_DEVICE_STATE_RESET_FLR  0x3
> #define PCI_DEVICE_STATE_RESET_MASK  0x3
> uint32_t flags;
> };

Yes, with the last #define adjusted such that columns align.

Jan

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-08-02 Thread Chen, Jiqian 
On 2024/8/2 14:25, Jan Beulich wrote:
> On 02.08.2024 04:55, Chen, Jiqian wrote:
>> On 2024/7/31 23:55, Roger Pau Monné wrote:
>>> On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
 When a device has been reset on dom0 side, the Xen hypervisor
 doesn'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 support the reset
 of pcidev and clear the vpci state of device. So that once the
 state of device is reset on dom0 side, dom0 can call this
 hypercall to notify hypervisor.

 Signed-off-by: Jiqian Chen 
 Signed-off-by: Huang Rui 
 Signed-off-by: Jiqian Chen 
 Reviewed-by: Stewart Hildebrand 
 Reviewed-by: Stefano Stabellini 
>>>
>>> Thanks, just a couple of nits.
>>>
>>> This is missing a changelog between versions, and I haven't been
>>> following all the versions, so some of my questions might have been
>>> answered in previous revisions.
>> Sorry, I will add changelogs here in next version.
>>
>>>
 ---
  xen/arch/x86/hvm/hypercall.c |  1 +
  xen/drivers/pci/physdev.c| 52 
  xen/drivers/vpci/vpci.c  | 10 +++
  xen/include/public/physdev.h | 16 +++
  xen/include/xen/vpci.h   |  8 ++
  5 files changed, 87 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..c0f47945d955 100644
 --- a/xen/drivers/pci/physdev.c
 +++ b/xen/drivers/pci/physdev.c
 @@ -2,6 +2,7 @@
  #include 
  #include 
  #include 
 +#include 
  
  #ifndef COMPAT
  typedef long ret_t;
 @@ -67,6 +68,57 @@ 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 pci_dev *pdev;
 +pci_sbdf_t sbdf;
 +
 +ret = -EOPNOTSUPP;
 +if ( !is_pci_passthrough_enabled() )
 +break;
 +
 +ret = -EFAULT;
 +if ( copy_from_guest(_reset, arg, 1) != 0 )
 +break;
 +
 +sbdf = PCI_SBDF(dev_reset.dev.seg,
 +dev_reset.dev.bus,
 +dev_reset.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(>domain->pci_lock);
 +pcidevs_unlock();
 +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;
 +
 +default:
 +ret = -EOPNOTSUPP;
 +break;
 +}
 +write_unlock(>domain->pci_lock);
 +
 +break;
 +}
 +
  default:
  ret = -ENOSYS;
  break;
 diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
 index 1e6aa5d799b9..7e914d1eff9f 100644
 --- a/xen/drivers/vpci/vpci.c
 +++ b/xen/drivers/vpci/vpci.c
 @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
  
  return rc;
  }
 +
 +int vpci_reset_device_state(struct pci_dev *pdev,
 +uint32_t reset_type)
>>>
>>> There's probably no use in passing reset_type to
>>> vpci_reset_device_state() if it's ignored?
>>>
 +{
 +ASSERT(rw_is_write_locked(>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..3cfde3fd2389

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-08-02 Thread Jan Beulich 
On 02.08.2024 04:55, Chen, Jiqian wrote:
> On 2024/7/31 23:55, Roger Pau Monné wrote:
>> On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
>>> When a device has been reset on dom0 side, the Xen hypervisor
>>> doesn'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 support the reset
>>> of pcidev and clear the vpci state of device. So that once the
>>> state of device is reset on dom0 side, dom0 can call this
>>> hypercall to notify hypervisor.
>>>
>>> Signed-off-by: Jiqian Chen 
>>> Signed-off-by: Huang Rui 
>>> Signed-off-by: Jiqian Chen 
>>> Reviewed-by: Stewart Hildebrand 
>>> Reviewed-by: Stefano Stabellini 
>>
>> Thanks, just a couple of nits.
>>
>> This is missing a changelog between versions, and I haven't been
>> following all the versions, so some of my questions might have been
>> answered in previous revisions.
> Sorry, I will add changelogs here in next version.
> 
>>
>>> ---
>>>  xen/arch/x86/hvm/hypercall.c |  1 +
>>>  xen/drivers/pci/physdev.c| 52 
>>>  xen/drivers/vpci/vpci.c  | 10 +++
>>>  xen/include/public/physdev.h | 16 +++
>>>  xen/include/xen/vpci.h   |  8 ++
>>>  5 files changed, 87 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..c0f47945d955 100644
>>> --- a/xen/drivers/pci/physdev.c
>>> +++ b/xen/drivers/pci/physdev.c
>>> @@ -2,6 +2,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  
>>>  #ifndef COMPAT
>>>  typedef long ret_t;
>>> @@ -67,6 +68,57 @@ 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 pci_dev *pdev;
>>> +pci_sbdf_t sbdf;
>>> +
>>> +ret = -EOPNOTSUPP;
>>> +if ( !is_pci_passthrough_enabled() )
>>> +break;
>>> +
>>> +ret = -EFAULT;
>>> +if ( copy_from_guest(_reset, arg, 1) != 0 )
>>> +break;
>>> +
>>> +sbdf = PCI_SBDF(dev_reset.dev.seg,
>>> +dev_reset.dev.bus,
>>> +dev_reset.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(>domain->pci_lock);
>>> +pcidevs_unlock();
>>> +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;
>>> +
>>> +default:
>>> +ret = -EOPNOTSUPP;
>>> +break;
>>> +}
>>> +write_unlock(>domain->pci_lock);
>>> +
>>> +break;
>>> +}
>>> +
>>>  default:
>>>  ret = -ENOSYS;
>>>  break;
>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>> index 1e6aa5d799b9..7e914d1eff9f 100644
>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
>>>  
>>>  return rc;
>>>  }
>>> +
>>> +int vpci_reset_device_state(struct pci_dev *pdev,
>>> +uint32_t reset_type)
>>
>> There's probably no use in passing reset_type to
>> vpci_reset_device_state() if it's ignored?
>>
>>> +{
>>> +ASSERT(rw_is_write_locked(>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..3cfde3fd2389 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

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-08-01 Thread Chen, Jiqian 
On 2024/8/1 14:49, Jan Beulich wrote:
> On 31.07.2024 18:13, Roger Pau Monné wrote:
>> On Wed, Jul 31, 2024 at 05:58:54PM +0200, Jan Beulich wrote:
>>> On 31.07.2024 17:55, Roger Pau Monné wrote:
 On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
>  
>  return rc;
>  }
> +
> +int vpci_reset_device_state(struct pci_dev *pdev,
> +uint32_t reset_type)

 There's probably no use in passing reset_type to
 vpci_reset_device_state() if it's ignored?
>>>
>>> I consider this forward-looking. It seems rather unlikely that in the
>>> longer run the reset type doesn't matter.
>>
>> I'm fine with having it in the hypercall interface, but passing it to
>> vpci_reset_device_state() can be done once there's a purpose for it,
>> and it won't change any public facing interface.
> 
> Jiqian, just to clarify: I'm okay either way.
Thank you very much! You dispelled my concerns.
I will remove reset_type in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-08-01 Thread Chen, Jiqian 
On 2024/7/31 23:55, Roger Pau Monné wrote:
> On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
>> When a device has been reset on dom0 side, the Xen hypervisor
>> doesn'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 support the reset
>> of pcidev and clear the vpci state of device. So that once the
>> state of device is reset on dom0 side, dom0 can call this
>> hypercall to notify hypervisor.
>>
>> Signed-off-by: Jiqian Chen 
>> Signed-off-by: Huang Rui 
>> Signed-off-by: Jiqian Chen 
>> Reviewed-by: Stewart Hildebrand 
>> Reviewed-by: Stefano Stabellini 
> 
> Thanks, just a couple of nits.
> 
> This is missing a changelog between versions, and I haven't been
> following all the versions, so some of my questions might have been
> answered in previous revisions.
Sorry, I will add changelogs here in next version.

> 
>> ---
>>  xen/arch/x86/hvm/hypercall.c |  1 +
>>  xen/drivers/pci/physdev.c| 52 
>>  xen/drivers/vpci/vpci.c  | 10 +++
>>  xen/include/public/physdev.h | 16 +++
>>  xen/include/xen/vpci.h   |  8 ++
>>  5 files changed, 87 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..c0f47945d955 100644
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -2,6 +2,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #ifndef COMPAT
>>  typedef long ret_t;
>> @@ -67,6 +68,57 @@ 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 pci_dev *pdev;
>> +pci_sbdf_t sbdf;
>> +
>> +ret = -EOPNOTSUPP;
>> +if ( !is_pci_passthrough_enabled() )
>> +break;
>> +
>> +ret = -EFAULT;
>> +if ( copy_from_guest(_reset, arg, 1) != 0 )
>> +break;
>> +
>> +sbdf = PCI_SBDF(dev_reset.dev.seg,
>> +dev_reset.dev.bus,
>> +dev_reset.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(>domain->pci_lock);
>> +pcidevs_unlock();
>> +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;
>> +
>> +default:
>> +ret = -EOPNOTSUPP;
>> +break;
>> +}
>> +write_unlock(>domain->pci_lock);
>> +
>> +break;
>> +}
>> +
>>  default:
>>  ret = -ENOSYS;
>>  break;
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 1e6aa5d799b9..7e914d1eff9f 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
>>  
>>  return rc;
>>  }
>> +
>> +int vpci_reset_device_state(struct pci_dev *pdev,
>> +uint32_t reset_type)
> 
> There's probably no use in passing reset_type to
> vpci_reset_device_state() if it's ignored?
> 
>> +{
>> +ASSERT(rw_is_write_locked(>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..3cfde3fd2389 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

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-08-01 Thread Jan Beulich 
On 31.07.2024 18:13, Roger Pau Monné wrote:
> On Wed, Jul 31, 2024 at 05:58:54PM +0200, Jan Beulich wrote:
>> On 31.07.2024 17:55, Roger Pau Monné wrote:
>>> On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
 --- a/xen/drivers/vpci/vpci.c
 +++ b/xen/drivers/vpci/vpci.c
 @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
  
  return rc;
  }
 +
 +int vpci_reset_device_state(struct pci_dev *pdev,
 +uint32_t reset_type)
>>>
>>> There's probably no use in passing reset_type to
>>> vpci_reset_device_state() if it's ignored?
>>
>> I consider this forward-looking. It seems rather unlikely that in the
>> longer run the reset type doesn't matter.
> 
> I'm fine with having it in the hypercall interface, but passing it to
> vpci_reset_device_state() can be done once there's a purpose for it,
> and it won't change any public facing interface.

Jiqian, just to clarify: I'm okay either way.

Jan

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-07-31 Thread Roger Pau Monné 
On Wed, Jul 31, 2024 at 05:58:54PM +0200, Jan Beulich wrote:
> On 31.07.2024 17:55, Roger Pau Monné wrote:
> > On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
> >> --- a/xen/drivers/vpci/vpci.c
> >> +++ b/xen/drivers/vpci/vpci.c
> >> @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
> >>  
> >>  return rc;
> >>  }
> >> +
> >> +int vpci_reset_device_state(struct pci_dev *pdev,
> >> +uint32_t reset_type)
> > 
> > There's probably no use in passing reset_type to
> > vpci_reset_device_state() if it's ignored?
> 
> I consider this forward-looking. It seems rather unlikely that in the
> longer run the reset type doesn't matter.

I'm fine with having it in the hypercall interface, but passing it to
vpci_reset_device_state() can be done once there's a purpose for it,
and it won't change any public facing interface.

Thanks, Roger.

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-07-31 Thread Jan Beulich 
On 31.07.2024 17:55, Roger Pau Monné wrote:
> On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
>>  
>>  return rc;
>>  }
>> +
>> +int vpci_reset_device_state(struct pci_dev *pdev,
>> +uint32_t reset_type)
> 
> There's probably no use in passing reset_type to
> vpci_reset_device_state() if it's ignored?

I consider this forward-looking. It seems rather unlikely that in the
longer run the reset type doesn't matter.

Jan

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-07-31 Thread Roger Pau Monné 
On Mon, Jul 08, 2024 at 07:41:18PM +0800, Jiqian Chen wrote:
> When a device has been reset on dom0 side, the Xen hypervisor
> doesn'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 support the reset
> of pcidev and clear the vpci state of device. So that once the
> state of device is reset on dom0 side, dom0 can call this
> hypercall to notify hypervisor.
> 
> Signed-off-by: Jiqian Chen 
> Signed-off-by: Huang Rui 
> Signed-off-by: Jiqian Chen 
> Reviewed-by: Stewart Hildebrand 
> Reviewed-by: Stefano Stabellini 

Thanks, just a couple of nits.

This is missing a changelog between versions, and I haven't been
following all the versions, so some of my questions might have been
answered in previous revisions.

> ---
>  xen/arch/x86/hvm/hypercall.c |  1 +
>  xen/drivers/pci/physdev.c| 52 
>  xen/drivers/vpci/vpci.c  | 10 +++
>  xen/include/public/physdev.h | 16 +++
>  xen/include/xen/vpci.h   |  8 ++
>  5 files changed, 87 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..c0f47945d955 100644
> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -2,6 +2,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #ifndef COMPAT
>  typedef long ret_t;
> @@ -67,6 +68,57 @@ 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 pci_dev *pdev;
> +pci_sbdf_t sbdf;
> +
> +ret = -EOPNOTSUPP;
> +if ( !is_pci_passthrough_enabled() )
> +break;
> +
> +ret = -EFAULT;
> +if ( copy_from_guest(_reset, arg, 1) != 0 )
> +break;
> +
> +sbdf = PCI_SBDF(dev_reset.dev.seg,
> +dev_reset.dev.bus,
> +dev_reset.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(>domain->pci_lock);
> +pcidevs_unlock();
> +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;
> +
> +default:
> +ret = -EOPNOTSUPP;
> +break;
> +}
> +write_unlock(>domain->pci_lock);
> +
> +break;
> +}
> +
>  default:
>  ret = -ENOSYS;
>  break;
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 1e6aa5d799b9..7e914d1eff9f 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
>  
>  return rc;
>  }
> +
> +int vpci_reset_device_state(struct pci_dev *pdev,
> +uint32_t reset_type)

There's probably no use in passing reset_type to
vpci_reset_device_state() if it's ignored?

> +{
> +ASSERT(rw_is_write_locked(>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..3cfde3fd2389 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;
> @@ -305,6 +312,15 @@

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-07-09 Thread Jan Beulich 
On 09.07.2024 04:47, Chen, Jiqian wrote:
> On 2024/7/8 22:56, Jan Beulich wrote:
>> On 08.07.2024 13:41, Jiqian Chen wrote:
>>> When a device has been reset on dom0 side, the Xen hypervisor
>>> doesn'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 support the reset
>>> of pcidev and clear the vpci state of device. So that once the
>>> state of device is reset on dom0 side, dom0 can call this
>>> hypercall to notify hypervisor.
>>>
>>> Signed-off-by: Jiqian Chen 
>>> Signed-off-by: Huang Rui 
>>> Signed-off-by: Jiqian Chen 
>>> Reviewed-by: Stewart Hildebrand 
>>> Reviewed-by: Stefano Stabellini 
>>
>> Reviewed-by: Jan Beulich 
> Thank you very much!
> 
>>
>> Just to double check: You're sure the other two R-b are still applicable,
>> despite the various changes that have been made?
> Will remove in next version.
> 
>>
>> As a purely cosmetic remark: I think I would have preferred if the new
>> identifiers didn't have "state" as a part; I simply don't think this adds
>> much value, while at the same time making these pretty long.
> Do you mean: remove "state" identifier on all the new codes?

"part of identifiers", yes. As that's a personal view, I wouldn't insist
though, unless others shared my perspective.

Jan

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-07-08 Thread Chen, Jiqian 
On 2024/7/8 22:56, Jan Beulich wrote:
> On 08.07.2024 13:41, Jiqian Chen wrote:
>> When a device has been reset on dom0 side, the Xen hypervisor
>> doesn'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 support the reset
>> of pcidev and clear the vpci state of device. So that once the
>> state of device is reset on dom0 side, dom0 can call this
>> hypercall to notify hypervisor.
>>
>> Signed-off-by: Jiqian Chen 
>> Signed-off-by: Huang Rui 
>> Signed-off-by: Jiqian Chen 
>> Reviewed-by: Stewart Hildebrand 
>> Reviewed-by: Stefano Stabellini 
> 
> Reviewed-by: Jan Beulich 
Thank you very much!

> 
> Just to double check: You're sure the other two R-b are still applicable,
> despite the various changes that have been made?
Will remove in next version.

> 
> As a purely cosmetic remark: I think I would have preferred if the new
> identifiers didn't have "state" as a part; I simply don't think this adds
> much value, while at the same time making these pretty long.
Do you mean: remove "state" identifier on all the new codes?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-07-08 Thread Jan Beulich 
On 08.07.2024 13:41, Jiqian Chen wrote:
> When a device has been reset on dom0 side, the Xen hypervisor
> doesn'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 support the reset
> of pcidev and clear the vpci state of device. So that once the
> state of device is reset on dom0 side, dom0 can call this
> hypercall to notify hypervisor.
> 
> Signed-off-by: Jiqian Chen 
> Signed-off-by: Huang Rui 
> Signed-off-by: Jiqian Chen 
> Reviewed-by: Stewart Hildebrand 
> Reviewed-by: Stefano Stabellini 

Reviewed-by: Jan Beulich 

Just to double check: You're sure the other two R-b are still applicable,
despite the various changes that have been made?

As a purely cosmetic remark: I think I would have preferred if the new
identifiers didn't have "state" as a part; I simply don't think this adds
much value, while at the same time making these pretty long.

Jan

[XEN PATCH v12 1/7] xen/pci: Add hypercall to support reset of pcidev

2024-07-08 Thread Jiqian Chen 
When a device has been reset on dom0 side, the Xen hypervisor
doesn'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 support the reset
of pcidev and clear the vpci state of device. So that once the
state of device is reset on dom0 side, dom0 can call this
hypercall to notify hypervisor.

Signed-off-by: Jiqian Chen 
Signed-off-by: Huang Rui 
Signed-off-by: Jiqian Chen 
Reviewed-by: Stewart Hildebrand 
Reviewed-by: Stefano Stabellini 
---
 xen/arch/x86/hvm/hypercall.c |  1 +
 xen/drivers/pci/physdev.c| 52 
 xen/drivers/vpci/vpci.c  | 10 +++
 xen/include/public/physdev.h | 16 +++
 xen/include/xen/vpci.h   |  8 ++
 5 files changed, 87 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..c0f47945d955 100644
--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -2,6 +2,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifndef COMPAT
 typedef long ret_t;
@@ -67,6 +68,57 @@ 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 pci_dev *pdev;
+pci_sbdf_t sbdf;
+
+ret = -EOPNOTSUPP;
+if ( !is_pci_passthrough_enabled() )
+break;
+
+ret = -EFAULT;
+if ( copy_from_guest(_reset, arg, 1) != 0 )
+break;
+
+sbdf = PCI_SBDF(dev_reset.dev.seg,
+dev_reset.dev.bus,
+dev_reset.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(>domain->pci_lock);
+pcidevs_unlock();
+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;
+
+default:
+ret = -EOPNOTSUPP;
+break;
+}
+write_unlock(>domain->pci_lock);
+
+break;
+}
+
 default:
 ret = -ENOSYS;
 break;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 1e6aa5d799b9..7e914d1eff9f 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -172,6 +172,16 @@ int vpci_assign_device(struct pci_dev *pdev)
 
 return rc;
 }
+
+int vpci_reset_device_state(struct pci_dev *pdev,
+uint32_t reset_type)
+{
+ASSERT(rw_is_write_locked(>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..3cfde3fd2389 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;
@@ -305,6 +312,15 @@ struct physdev_pci_device {
 typedef struct physdev_pci_device physdev_pci_device_t;
 DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t);
 
+struct pci_device_state_reset {
+physdev_pci_device_t dev;
+#define PCI_DEVICE_STATE_RESET_COLD 0
+#define PCI_DEVICE_STATE_RESET_WARM 1
+#define PCI_DEVICE_STATE_RESET_HOT  2
+#define PCI_DEVICE_STATE_RESET_FLR  3
+uint32_t reset_type;
+};
+
 #define PHYSDEVOP_DBGP_RESET_PREPARE1
 #define PHYSDEVOP_DBGP_RESET_DONE   2
 
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index da8d0f41e6f4..6be812dbc04a 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -38,6 +38,8