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.jime...@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
