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

Re: [PATCH v2 1/2] xen/vpci: header: filter PCI capabilities


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
  • Date: Sun, 20 Aug 2023 21:28:36 -0400
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=suse.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=QawFhNY1hP7c1fV/Uccd/vDghLmwuS2+B+Tf4M2K+r8=; b=ftdaNS5/H2c04ps8ubabURxhmYOF8xCTkf/Xc8TRp3kJyEZdbK2VeHiuHavAiQlPYgmcBl48JzMWhYiDkUgn8s1861CjEN+FyK2TFvIk2C+3XjAVXPCapTTnBkfnm2ijZ91F7M+v6AKeyMdv03pF2eSdcUhc7+8jOwtsgL6KeI8YJp/oVECufsZx/zN8y1X59XfShoRhZY0gVq/i48WBUfQQlrMzSVk9Zp64Tg1qHGnhpArV43ekLtr1TrvAmuVqdk29BEdsUZOb7G+rQhwp0KxkatWb+msUIxCgy/G6ffwYTRgxUJO22lAC0bH/obifV4IXpwviKmcxEpSgV8vF0w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ofyfXaaY1hZkpz/zJMRnfjWtI3qF6Ryw9lWtsFnFzbjzv9pDiHjGvW5L9+ll9vd5uxD46TYKSan37c0MWU0I/HP6UyXCtau2ATgZj4WkNK1PoSrkl64ua0VEpIn+8KrlcnSvUSiHqHUGde25y6jHTqvhMHoOC/1+9PMjXS1SM1A+hhyXWYHRmfODexO4wyT4KL+dvjo0rcsoxFtgmi+DZ1QkzbsErbSu71Yi4wtvYwY5xpnPRG2NEVn8s3sRKiOP+dEe94pgKIYwvf4b0fVMbaNvslo1kXr/CfcHdkSbQmSGVXXcd1hpb1HvK6a+x5AfupiFPWwgERXVB1XX4zLLrA==
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 21 Aug 2023 01:29:14 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 8/17/23 08:57, Jan Beulich wrote:
> On 16.08.2023 20:50, Stewart Hildebrand wrote:
>> If there are no capabilities to be exposed to the guest, a future status
>> register handler likely would want to mask the PCI_STATUS_CAP_LIST bit. See 
>> [1]
>> for a suggestion of how this might be tracked in struct vpci_header.
> 
> Can we actually get away without doing this right away? I'm not sure
> consumers are required to range check what they read from PCI_CAPABILITY_LIST.

I think you're right, this should be done right away. I'll add a status 
register handler with PCI_STATUS_CAP_LIST bit masking in the next version of 
the series. This will include a modification to vpci_write_helper() be able to 
handle rw1c registers properly.

>> --- a/xen/drivers/pci/pci.c
>> +++ b/xen/drivers/pci/pci.c
>> @@ -39,27 +39,27 @@ int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 
>> func, u8 cap)
>>      return 0;
>>  }
>>
>> -int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap)
>> +int pci_find_next_cap(pci_sbdf_t sbdf, uint8_t pos, bool 
>> (*is_match)(uint8_t),
>> +                      int *ttl)
> 
> Why plain int? When values can't go negative, respective variables generally
> want to be of unsigned types.

I'll change to unsigned int.

>>  {
>> -    u8 id;
>> -    int ttl = 48;
>> +    uint8_t id;
>>
>> -    while ( ttl-- )
>> +    while ( (*ttl)-- > 0 )
> 
> I don't see why you add "> 0" here.

I'll remove it.

>>      {
>> -        pos = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos);
>> +        pos = pci_conf_read8(sbdf, pos);
>>          if ( pos < 0x40 )
>>              break;
>>
>> -        pos &= ~3;
>> -        id = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos + 
>> PCI_CAP_LIST_ID);
>> +        id = pci_conf_read8(sbdf, (pos & ~3) + PCI_CAP_LIST_ID);
>>
>>          if ( id == 0xff )
>>              break;
>> -        if ( id == cap )
>> +        if ( is_match(id) )
>>              return pos;
>>
>> -        pos += PCI_CAP_LIST_NEXT;
>> +        pos = (pos & ~3) + PCI_CAP_LIST_NEXT;
>>      }
>> +
>>      return 0;
>>  }
> 
> As said in context of v1, this function should remain usable for its
> original purpose. That, to me, includes the caller not needing to care about
> ttl. I could see you convert the original function the way you do, but under
> a new name, and then implement the original one simply in terms of this more
> general purpose function.

Will do.

> Also, while I appreciate the sbdf conversion, that wants to be a separate
> patch, which would then want to take care of the sibling functions as well.

OK, will do. To be clear, this means converting the following 4 functions to 
use pci_sbdf_t, along with all the call sites:
pci_find_cap_offset
pci_find_next_cap
pci_find_ext_capability
pci_find_next_ext_capability

>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -513,6 +513,18 @@ static void cf_check rom_write(
>>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>>  }
>>
>> +static bool cf_check vpci_cap_supported(uint8_t id)
>> +{
>> +    switch ( id )
>> +    {
>> +    case PCI_CAP_ID_MSI:
>> +    case PCI_CAP_ID_MSIX:
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
>> +}
>> +
>>  static int cf_check init_bars(struct pci_dev *pdev)
>>  {
>>      uint16_t cmd;
>> @@ -544,6 +556,60 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>      if ( rc )
>>          return rc;
>>
>> +    if ( !is_hardware_domain(pdev->domain) )
>> +    {
>> +        if ( (pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST)
>> +             == 0 )
> 
> This fits on a single line when written this more commonly used way:
> 
>         if ( !(pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST) 
> )

I'll do this.

> Otherwise it needs wrapping differently - binary operators at a wrapping
> point belong on the earlier line in our style.
> 
>> +        {
>> +            /* RAZ/WI */
>> +            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>> +                                   PCI_CAPABILITY_LIST, 1, NULL);
> 
> This last NULL is likely misleading to readers: It does not obviously
> represent the value 0 cast to void *. (Same then for the extended cap
> handler at the end.)

I'll change to (void *)0

>> +            if ( rc )
>> +                return rc;
>> +        }
>> +        else
>> +        {
>> +            /* Only expose capabilities to the guest that vPCI can handle. 
>> */
>> +            uint8_t next;
>> +            int ttl = 48;
>> +
>> +            next = pci_find_next_cap(pdev->sbdf, PCI_CAPABILITY_LIST,
>> +                                     vpci_cap_supported, &ttl);
>> +
>> +            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>> +                                   PCI_CAPABILITY_LIST, 1,
>> +                                   (void *)(uintptr_t)next);
>> +            if ( rc )
>> +                return rc;
>> +
>> +            while ( next && (ttl > 0) )
> 
> Don't you need to mask off the low two bits first (rather than [only] ...

Yes, I'll fix this.

> 
>> +            {
>> +                uint8_t pos = next;
>> +
>> +                next = pci_find_next_cap(pdev->sbdf, pos + 
>> PCI_CAP_LIST_NEXT,
>> +                                         vpci_cap_supported, &ttl);
>> +
>> +                rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>> +                                       pos + PCI_CAP_LIST_ID, 1, NULL);
>> +                if ( rc )
>> +                    return rc;
>> +
>> +                rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>> +                                       pos + PCI_CAP_LIST_NEXT, 1,
>> +                                       (void *)(uintptr_t)next);
>> +                if ( rc )
>> +                    return rc;
>> +
>> +                next &= ~3;
> 
> ... here)?
> 
> Jan



 


Rackspace

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