[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 1/3] vpci: Hide capability when it fails to initialize


  • To: "Chen, Jiqian" <Jiqian.Chen@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 31 Mar 2025 10:46:31 +0200
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Huang, Ray" <Ray.Huang@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Mon, 31 Mar 2025 08:46:33 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 31.03.2025 10:43, Roger Pau Monné wrote:
> On Mon, Mar 31, 2025 at 07:26:20AM +0000, Chen, Jiqian wrote:
>> On 2025/3/27 17:25, Roger Pau Monné wrote:
>>> On Thu, Mar 27, 2025 at 03:32:12PM +0800, Jiqian Chen wrote:
>>>> --- a/xen/drivers/vpci/vpci.c
>>>> +++ b/xen/drivers/vpci/vpci.c
>>>> @@ -36,8 +36,8 @@ struct vpci_register {
>>>>  };
>>>>  
>>>>  #ifdef __XEN__
>>>> -extern vpci_register_init_t *const __start_vpci_array[];
>>>> -extern vpci_register_init_t *const __end_vpci_array[];
>>>> +extern vpci_capability_t *const __start_vpci_array[];
>>>> +extern vpci_capability_t *const __end_vpci_array[];
>>>>  #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>>>>  
>>>>  #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>> @@ -83,6 +83,47 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
>>>>  
>>>>  #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>>>>  
>>>> +static int vpci_init_cap_with_priority(struct pci_dev *pdev,
>>>> +                                       const char *priority)
>>>> +{
>>>> +    for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
>>>> +    {
>>>> +        const vpci_capability_t *capability = __start_vpci_array[i];
>>>> +        const unsigned int cap_id = capability->id;
>>>> +        unsigned int pos;
>>>> +        int rc;
>>>> +
>>>> +        if ( *(capability->priority) != *priority )
>>>> +            continue;
>>>> +
>>>> +        if ( !capability->is_ext )
>>>> +            pos = pci_find_cap_offset(pdev->sbdf, cap_id);
>>>> +        else
>>>> +            pos = pci_find_ext_capability(pdev->sbdf, cap_id);
>>>> +
>>>> +        if ( !pos )
>>>> +            continue;
>>>> +
>>>> +        rc = capability->init(pdev);
>>>> +
>>>> +        if ( rc )
>>>> +        {
>>>> +            printk(XENLOG_WARNING "%pd %pp: cap init fail rc=%d, try to 
>>>> hide\n",
>>>> +                   pdev->domain, &pdev->sbdf, rc);
>>>> +            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>>>> +                                   pos, capability->is_ext ? 4 : 1, NULL);
>>>
>>> Are you sure this works as intended? 
>> Yes, I used failure test cases of init_msi/rebar.
>> From the "lspci" result, they were hided from the dom0.
>> But I forgot to test for domUs.
> 
> I assume that's only tested with Linux?  See my comment below about
> capability ID 0 being reserved, and hence I think we should not keep
> capabilities with ID 0 on the list, as it might cause malfunctions to
> OSes.
> 
>>> The capability ID 0 is marked as "reserved" in the spec, so it's unclear to 
>>> me how OSes would handle
>>> finding such capability on the list - I won't be surprised if some
>>> implementations decide to terminate the walk.  It's fine to mask the
>>> capability ID for the ones that we don't want to expose, but there's
>>> further work to do IMO.
>>>
>>> The usual way to deal with masking capabilities is to short circuit
>>> the capability from the linked list, by making the previous capability
>>> "Next Capability Offset" point to the next capability in the list,
>>> thus skipping the current one. So:
>>>
>>> capability[n - 1].next_cap = capability[n].next_cap
>>>
>>> IOW: you will need to add the handler to the previous capability on
>>> the list.  That's how it's already done in init_header().
>> Oh, I got your opinion.
>> But we may need to discuss this more.
>> In my opinion, there should be two situations:
>> First, if device belongs to hardware domain, there is no emulation of legacy 
>> or extended capabilities linked list if I understand codes right.
> 
> Yes, but that needs to be fixed, we need to have this kind of
> emulation uniformly.
> 
>> So, for this situation, I think current implementation of my patch is enough 
>> for hiding legacy or extended capabilities.
> 
> It works given the current code in Linux.  As said above, I don't
> think this is fully correct according to the PCI spec.
> 
>> Second, if device belongs to common domain, we just need to consider legacy 
>> capabilities since all extended capabilities are hided in init_header().
>> So, for this situation, I need to what you said " capability[n - 1].next_cap 
>> = capability[n].next_cap "
> 
> I'm not sure why would want to handle the hardware domain vs
> unprivileged domains differently here.  The way to hide the
> capabilities should always be the same, like it's currently done for
> domUs.

FWIW - I agree with Roger on all the point made.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.