Hi Daniel P. Smith,
On 2023/11/30 22:52, Roger Pau Monné wrote:
> On Thu, Nov 30, 2023 at 07:39:38AM -0500, Daniel P. Smith wrote:
>> On 11/30/23 07:25, Daniel P. Smith wrote:
>>> On 11/30/23 01:22, Chen, Jiqian wrote:
>>>> Hi Roger and Daniel P. Smith,
>>>>
>>>> On 2023/11/28 22:08, Roger Pau Monné wrote:
>>>>> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote:
>>>>>> 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, this patch add new hypercall to clear
>>>>>> all vpci device state. And when reset device happens on dom0
>>>>>> side, dom0 can call this hypercall to notify vpci.
>>>>>>
>>>>>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>>>>>> Signed-off-by: Huang Rui <ray.hu...@amd.com>
>>>>>> ---
>>>>>> xen/arch/x86/hvm/hypercall.c | 1 +
>>>>>> xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++
>>>>>> xen/drivers/pci/physdev.c | 14 ++++++++++++++
>>>>>> xen/drivers/vpci/vpci.c | 9 +++++++++
>>>>>> xen/include/public/physdev.h | 2 ++
>>>>>> xen/include/xen/pci.h | 1 +
>>>>>> xen/include/xen/vpci.h | 6 ++++++
>>>>>> 7 files changed, 54 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/x86/hvm/hypercall.c
>>>>>> b/xen/arch/x86/hvm/hypercall.c
>>>>>> index eeb73e1aa5..6ad5b4d5f1 100644
>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>> @@ -84,6 +84,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/passthrough/pci.c
>>>>>> b/xen/drivers/passthrough/pci.c
>>>>>> index 04d00c7c37..f871715585 100644
>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>>>>> return ret;
>>>>>> }
>>>>>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn)
>>>>>
>>>>> You could use pci_sbdf_t here instead of 3 parameters.
>>>> Will change in next version, thank you.
>>>>
>>>>>
>>>>> I'm however unsure whether we really need this helper just to fetch
>>>>> the pdev and call vpci_reset_device_state(), I think you could place
>>>>> this logic directly in pci_physdev_op(). Unless there are plans to
>>>>> use such logic outside of pci_physdev_op().
>>>> If I place the logic of pci_reset_device_state directly in
>>>> pci_physdev_op. I think I need to declare vpci_reset_device_state in
>>>> pci.h? If it is suitable, I will change in next version.
>>>>
>>>>>
>>>>>> +{
>>>>>> + struct pci_dev *pdev;
>>>>>> + int ret = -ENODEV;
>>>>>
>>>>> Some XSM check should be introduced fro this operation, as none of the
>>>>> existing ones is suitable. See xsm_resource_unplug_pci() for example.
>>>>>
>>>>> xsm_resource_reset_state_pci() or some such I would assume.
>>>> I don't know what I should do in XSM function(assume it is
>>>> xsm_resource_reset_state_pci).
>>>> Hi Daniel P. Smith, could you please give me some suggestions?
>>>> Just to check the XSM_PRIV action?
>>>>
>>>
>>> Roger, thank you for seeing this and adding me in!
>>>
>>> Jiqian, I just wanted to let you know I have seen your post but I have
>>> been a little tied up this week. Just with the cursory look, I think
>>> Roger is suggesting a new XSM check/hook is warranted.
>>>
>>> If you would like to attempt at writing the dummy policy side, mimic
>>> xsm_resource_plug_pci in xen/include/xsm/dummy.h and
>>> xen/include/xsm/xsm.h, then I can look at handling the FLASK portion
>>> next week and provide it to you for inclusion into the series. If you
>>> are not comfortable with it, I can look at the whole thing next week.
>>> Just let me know what you would be comfortable with.
>>
>> Apologies, thinking about for a moment and was thinking the hook should be
>> called xsm_resource_config_pci. I would reset as a config operation and
>> there might be new ones in the future. I do not believe there is a need to
>> have fine grain access control down to individual config operation, thus
>> they could all be captured under this one hook. Roger, what are your
>> thoughts about this, in particular how you see vpci evolving?
>
> So the configuration space reset should only be done by the domain
> that's also capable of triggering the physical device reset, usually
> the hardware domain. I don't think it's possible ATM to allow a
> domain different than the hardware domain to perform a PCI reset, as
> doing it requires unmediated access to the PCI config space.
>
> So resetting the vPCI state should either be limited to the hardware
> domain, or to a pci reset capability explicitly
> (xsm_resource_reset_pci or some such?). Or maybe
> xsm_resource_config_pci if that denotes unmediated access to the PCI
> config space.
>
> Thanks, Roger.
Is it like below that I need to add for XSM?
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 9d7519eb89..81ceaf145f 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -937,6 +937,10 @@ int pci_reset_device_state(u16 seg, u8 bus, u8 devfn)
struct pci_dev *pdev;
int ret = -ENODEV;
+ ret = xsm_resource_config_pci(XSM_PRIV, (seg << 16) | (bus << 8) | devfn);
+ if ( ret )
+ return ret;
+
pcidevs_lock();
pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn));
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 8671af1ba4..bcaff99b23 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -472,6 +472,13 @@ static XSM_INLINE int cf_check xsm_resource_setup_pci(
return xsm_default_action(action, current->domain, NULL);
}
+static XSM_INLINE int cf_check xsm_resource_config_pci(
+ XSM_DEFAULT_ARG uint32_t machine_bdf)
+{
+ XSM_ASSERT_ACTION(XSM_PRIV);
+ return xsm_default_action(action, current->domain, NULL);
+}
+
static XSM_INLINE int cf_check xsm_resource_setup_gsi(XSM_DEFAULT_ARG int gsi)
{
XSM_ASSERT_ACTION(XSM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 8dad03fd3d..1cb16b00de 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -571,6 +571,12 @@ static inline int xsm_resource_setup_pci(
return alternative_call(xsm_ops.resource_setup_pci, machine_bdf);
}
+static inline int xsm_resource_config_pci(
+ xsm_default_t def, uint32_t machine_bdf)
+{
+ return alternative_call(xsm_ops.resource_config_pci, machine_bdf);
+}
+
static inline int xsm_resource_setup_gsi(xsm_default_t def, int gsi)
{
return alternative_call(xsm_ops.resource_setup_gsi, gsi);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index e6ffa948f7..7a289ba5d8 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -91,6 +91,7 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops
= {
.resource_plug_pci = xsm_resource_plug_pci,
.resource_unplug_pci = xsm_resource_unplug_pci,
.resource_setup_pci = xsm_resource_setup_pci,
+ .resource_config_pci = xsm_resource_config_pci,
.resource_setup_gsi = xsm_resource_setup_gsi,
--
Best regards,
Jiqian Chen.
