On Sun, 2024-12-01 at 22:28 -0700, Alex Williamson wrote:
> CAUTION: External Email!!
> On Mon, 2 Dec 2024 00:09:32 +0800
> Tomita Moeko <tomitamo...@gmail.com> wrote:
>
> > Add helper functions igd_gtt_memory_size() and igd_stolen_size() for
> > calculating GTT stolen memory and Data stolen memory size in bytes,
> > and use macros to replace the hardware-related magic numbers for
> > better readability.
> >
> > Signed-off-by: Tomita Moeko <tomitamo...@gmail.com>
> > ---
> > hw/vfio/igd.c | 101 ++++++++++++++++++++++++++++-------------------
> > ---
> > 1 file changed, 57 insertions(+), 44 deletions(-)
> >
> > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> > index e40e601026..b449316ec0 100644
> > --- a/hw/vfio/igd.c
> > +++ b/hw/vfio/igd.c
> > @@ -116,6 +116,54 @@ typedef struct VFIOIGDQuirk {
> > #define IGD_BDSM 0x5c /* Base Data of Stolen Memory */
> > #define IGD_BDSM_GEN11 0xc0 /* Base Data of Stolen Memory of gen 11
> > and later */
> >
> > +#define IGD_GMCH_GEN6_GMS_SHIFT 3
> > +#define IGD_GMCH_GEN6_GMS_MASK 0x1f
> > +#define IGD_GMCH_GEN6_GGMS_SHIFT 8
> > +#define IGD_GMCH_GEN6_GGMS_MASK 0x3
> > +#define IGD_GMCH_GEN8_GMS_SHIFT 8
> > +#define IGD_GMCH_GEN8_GMS_MASK 0xff
> > +#define IGD_GMCH_GEN8_GGMS_SHIFT 6
> > +#define IGD_GMCH_GEN8_GGMS_MASK 0x3
> > +You should consider reusing the names used by Linux [1]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/intel/i915_drm.h?h=v6.12#n50 > > +static uint64_t igd_gtt_memory_size(int gen, uint16_t gmch) > > +{ > > + uint64_t ggms; > > + > > + if (gen < 8) { > > + ggms = (gmch >> IGD_GMCH_GEN6_GGMS_SHIFT) & > > IGD_GMCH_GEN6_GGMS_MASK; > > + } else { > > + ggms = (gmch >> IGD_GMCH_GEN8_GGMS_SHIFT) & > > IGD_GMCH_GEN8_GGMS_MASK; > > + ggms *= 2; > > + } > > + > > + return ggms * MiB; > > +} > > + > > +static uint64_t igd_stolen_memory_size(int gen, uint32_t gmch) > > +{ > > + uint64_t gms; > > + > > + if (gen < 8) { > > + gms = (gmch >> IGD_GMCH_GEN6_GMS_SHIFT) & > > IGD_GMCH_GEN6_GMS_MASK; > > + } else { > > + gms = (gmch >> IGD_GMCH_GEN8_GMS_SHIFT) & > > IGD_GMCH_GEN8_GMS_MASK; > > + } > > + > > + if (gen < 9) { > > + if (gms < 0x10) { > > + return gms * 32 * MiB; > > + } > > igd_get_stolen_mb() used: > > if (gms > 0x10) { > error_report("Unsupported IGD GMS value 0x%x", gms); > return 0; > } > return gms * 32; > > Therefore this should use <= to be equivalent. I'd personally prefer moving the code in one commit and change the format in a second one. If you want to touch this, I'd align it with Linux, which multiplies gms by 32 MB unconditionally for gen 6 and 8 [1]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v6.12#n404 > > + } else { > > + if (gms < 0xf0) { > > + return gms * 32 * MiB; > > + } else { > > + return (gms - 0xf0 + 1) * 4 * MiB; > > This does not match the line from igd_get_stolen_mb(): > > return (gms - 0xf0) * 4 + 4; > > Explain. > I've chosen the current format because it aligns with Linux [1]. I would keep it as is to make it easier to sync with Linux. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/early-quirks.c?h=v6.12#n460 > > + } > > + } > > + > > + error_report("Unsupported IGD GMS value 0x%lx", gms); > > + return 0; > > +} > > > > /* > > * The rather short list of registers that we copy from the host > > devices. > > @@ -264,17 +312,10 @@ static int vfio_pci_igd_lpc_init(VFIOPCIDevice > > *vdev, > > static int vfio_igd_gtt_max(VFIOPCIDevice *vdev) > > { > > uint32_t gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, > > sizeof(gmch)); > > - int ggms, gen = igd_gen(vdev); > > - > > - gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, > > sizeof(gmch)); > > - ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3; > > - if (gen > 7) { > > - ggms = 1 << ggms; > > - } > > - > > - ggms *= MiB; > > + int gen = igd_gen(vdev); > > + uint64_t ggms_size = igd_gtt_memory_size(gen, gmch); > > > > - return (ggms / (4 * KiB)) * (gen < 8 ? 4 : 8); > > + return (ggms_size / (4 * KiB)) * (gen < 8 ? 4 : 8); > > } > > > > /* > > @@ -481,30 +522,6 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice > > *vdev, int nr) > > QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next); > > } > > > > -static int igd_get_stolen_mb(int gen, uint32_t gmch) > > -{ > > - int gms; > > - > > - if (gen < 8) { > > - gms = (gmch >> 3) & 0x1f; > > - } else { > > - gms = (gmch >> 8) & 0xff; > > - } > > - > > - if (gen < 9) { > > - if (gms > 0x10) { > > - error_report("Unsupported IGD GMS value 0x%x", gms); > > - return 0; > > - } > > - return gms * 32; > > - } else { > > - if (gms < 0xf0) > > - return gms * 32; > > - else > > - return (gms - 0xf0) * 4 + 4; > > - } > > -} > > - > > void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr) > > { > > g_autofree struct vfio_region_info *rom = NULL; > > @@ -514,7 +531,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice > > *vdev, int nr) > > VFIOQuirk *quirk; > > VFIOIGDQuirk *igd; > > PCIDevice *lpc_bridge; > > - int i, ret, ggms_mb, gms_mb = 0, gen; > > + int i, ret, gen; > > + uint64_t ggms_size, gms_size; > > uint64_t *bdsm_size; > > uint32_t gmch; > > uint16_t cmd_orig, cmd; > > @@ -676,13 +694,8 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice > > *vdev, int nr) > > > > QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next); > > > > - /* Determine the size of stolen memory needed for GTT */ > > - ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3; > > - if (gen > 7) { > > - ggms_mb = 1 << ggms_mb; > > - } > > - > > - gms_mb = igd_get_stolen_mb(gen, gmch); > > + ggms_size = igd_gtt_memory_size(gen, gmch); > > + gms_size = igd_stolen_memory_size(gen, gmch); > > > > /* > > * Request reserved memory for stolen memory via fw_cfg. VM > > firmware > > @@ -693,7 +706,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice > > *vdev, int nr) > > * config offset 0x5C. > > */ > > bdsm_size = g_malloc(sizeof(*bdsm_size)); > > - *bdsm_size = cpu_to_le64((ggms_mb + gms_mb) * MiB); > > + *bdsm_size = cpu_to_le64(ggms_size + gms_size); > > fw_cfg_add_file(fw_cfg_find(), "etc/igd-bdsm-size", > > bdsm_size, sizeof(*bdsm_size)); > > > > @@ -744,5 +757,5 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice > > *vdev, int nr) > > vdev- > > >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e > > If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN- > > EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY > > Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 ); > > } > > > > - trace_vfio_pci_igd_bdsm_enabled(vdev- > > >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e > > If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN- > > EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY > > Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 , ggms_mb + gms_mb); > > + trace_vfio_pci_igd_bdsm_enabled(vdev- > > >https://nospamproxywebp.beckhoff.com/enQsig/link?id=BAgAAAD3YuKN4_e > > If2wAAADsWKVxcuZtvheRgRqIemuzgT-OABVjIqlMqKN- > > EU30bxV28qXZ_Y5_SMs0YfH8q_KtHOPLp1QG1mJuzWERjIVCEePSftEKzXou_Qr3jboY > > Ii_VUdvptqZTIPyfY-GJ9iOrnUrKoc_oifOB9wY1 , *bdsm_size / MiB); > > Technically this should use le64_to_cpu(*bdsm_size), or retain the > addition logic. Thanks, > > Alex -- Kind regards, Corvin disable-disclaimer-BADE
signature.asc
Description: This is a digitally signed message part
