Re: [PATCH 02/18] amd_iommu: Add helper function to extract the DTE

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Apr 16, 2025 at 09:29:23AM -0400, Alejandro Jimenez wrote:
> 
> 
> On 4/16/25 7:36 AM, Sairaj Kodilkar wrote:
> > 
> > 
> > On 4/14/2025 7:32 AM, Alejandro Jimenez wrote:
> > Hi Alejandro,
> > 
> > > Extracting the DTE from a given AMDVIAddressSpace pointer structure is a
> > > common operation required for syncing the shadow page tables. Implement a
> > > helper to do it and check for common error conditions.
> > > 
> > > Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> > > ---
> > >   hw/i386/amd_iommu.c | 47 ++++++++++++++++++++++++++++++++++++++++-----
> > >   1 file changed, 42 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> > > index 5f9b95279997..22d648c2e0e3 100644
> > > --- a/hw/i386/amd_iommu.c
> > > +++ b/hw/i386/amd_iommu.c
> > > @@ -77,6 +77,20 @@ typedef struct AMDVIIOTLBEntry {
> > >       uint64_t page_mask;         /* physical page size  */
> > >   } AMDVIIOTLBEntry;
> > > +/*
> > > + * These 'fault' reasons have an overloaded meaning since they are
> > > not only
> > > + * intended for describing reasons that generate an IO_PAGE_FAULT
> > > as per the AMD
> > > + * IOMMU specification, but are also used to signal internal errors
> > > in the
> > > + * emulation code.
> > > + */
> > > +typedef enum AMDVIFaultReason {
> > > +    AMDVI_FR_DTE_RTR_ERR = 1,           /* Failure to retrieve DTE */
> > > +    AMDVI_FR_DTE_V,                     /* DTE[V] = 0 */
> > > +    AMDVI_FR_DTE_TV,                    /* DTE[TV] = 0 */
> > > +} AMDVIFaultReason;
> > > +
> > > +static int amdvi_as_to_dte(AMDVIAddressSpace *as, uint64_t *dte);
> > > +
> > 
> > No need to have this function declaration as it is a static function.
> > 
> 
> I am adding a forward declaration since this function will be used by
> several other new and existing functions throughout the series, and this
> avoids having to keep moving the function definition. I do the same for many
> other functions in later patches, since it is more practical than constantly
> moving definitions around to guarantee ordering constraints. That approach
> would create patches with large diffs but non-functional changes due to code
> movement alone, makes it harder to parse the changes that actually matter,
> harder to rebase and resolve conflicts, etc...
> 
> Alejandro

You can add forward declarations temporarily to simplify review.  But in
the end, I prefer seeing code without forward declarations, with
functions ordered sensibly by order of calls, rather than spread
randomly all over the place.



> > 
> > Regards
> > Sairaj Kodilkar