[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
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |