Re: [PATCH v6 1/1] rust: pci: add extended capability and SR-IOV support

[Zhi Wang]

On Thu, 30 Jul 2026 19:45:05 +0100
"Gary Guo" <gary@xxxxxxxxxxx> wrote:

snip

> > +
> > +impl ConfigSpace<'_, ExtSriovRegs> {
> > +    /// Reads and decodes the VF memory BAR at configuration-space
> > slot `bar_index`.
> > +    #[inline]
> > +    pub fn read_vf_bar(&self, bar_index: usize) ->
> > Result<ExtSriovVfBar> {
> 
> Do you expect people to pass in a random index instead of 0 or
> next_index? If not, this should be an iterator. Otherwise it'd be
> possible to index into high part of 64-bit address.
> 

Yes, that is exactly what I want to avoid. Looking at the driver again,
it does not need random access, and I was thinking to make the
interface more convenient for the driver and avoid ambiguous numbers
from driver. Alex also suggested an iterator in an earlier review. So
having an interator makes more sense. I will rework this into an
iterator in the next re-spin.

Z.

> > +        if bar_index >= NUM_VF_BARS {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        let low = crate::io_read!(*self, .vf_bar[try: bar_index]);
> 
> Given the bound checking above I'd use `panic: ` here.
> 
> > +        if low & bindings::PCI_BASE_ADDRESS_SPACE !=
> > bindings::PCI_BASE_ADDRESS_SPACE_MEMORY {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        let is_64bit = low &
> > bindings::PCI_BASE_ADDRESS_MEM_TYPE_MASK
> > +            == bindings::PCI_BASE_ADDRESS_MEM_TYPE_64;
> > +        let following_index =
> > bar_index.checked_add(1).ok_or(EINVAL)?;
> 
> Just use operator here as it cannot overflow.
> 
> > +
> > +        let (address, next_index) = if is_64bit {
> > +            if following_index >= NUM_VF_BARS {
> > +                return Err(EINVAL);
> > +            }
> > +
> > +            let high = crate::io_read!(*self, .vf_bar[try:
> > following_index]);
> 
> Same here.
> 
> > +            (
> > +                (u64::from(high) << 32) | u64::from(low &
> > !VF_MEMORY_BAR_ATTRIBUTE_BITS),
> > +                following_index.checked_add(1).ok_or(EINVAL)?,
> 
> Same here.
> 
> Best,
> Gary
> 
> > +            )
> > +        } else {
> > +            (
> > +                u64::from(low & !VF_MEMORY_BAR_ATTRIBUTE_BITS),
> > +                following_index,
> > +            )
> > +        };
> > +
> > +        Ok(ExtSriovVfBar {
> > +            address,
> > +            is_64bit,
> > +            next_index,
> > +        })
> > +    }
> > +}
> 
>