Re: [PATCH v5 6/7] xen/riscv: page table handling

[oleksii.kurochko@xxxxxxxxx]

On Thu, 2024-08-29 at 14:14 +0200, Jan Beulich wrote:
> > > > >  Also note that "`mfn` is
> > > > > valid" isn't the same as "mfn != INVALID_MFN". You want to be
> > > > > precise
> > > > > here,
> > > > > to avoid confusion later on. (I say that knowing that we're
> > > > > still
> > > > > fighting
> > > > > especially shadow paging code on x86 not having those
> > > > > properly
> > > > > separated.)
> > > > If it is needed to be precise and mfn is valid isn't the same
> > > > as
> > > > "mfn
> > > > != INVALID_MFN" only for the case of shadow paging?
> > > 
> > > No, I used shadow paging only as an example of where we have
> > > similar
> > > issues. I'd like to avoid that a new port starts out with
> > > introducing
> > > more instances of that. You want to properly separate INVALID_MFN
> > > from
> > > "invalid MFN", where the latter means any MFN where either
> > > nothing
> > > exists at all, or (see mfn_valid()) where no struct page_info
> > > exists.
> > Well, now I think I understand the difference between "INVALID_MFN"
> > and
> > "invalid MFN."
> > 
> > Referring back to your original reply, I need to update the comment
> > above pt_update():
> > ```
> >     ...
> >       * If `mfn` is valid ( exist ) and flags has PTE_VALID bit set
> > then it
> >     means that inserting will be done.
> > ```
> > Would this be correct and more precise?
> 
> That depends on whether it correctly describes what the code does. If
> the code continues to check against INVALID_MFN, such a description
> wouldn't be correct. Also, just to re-iterate, ...
> 
> > Based on the code for mfn_valid(), the separation is currently done
> > using the max_page value, which can't be initialized at the moment
> > as
> > it requires reading the device tree file to obtain the RAM end.
> 
> ... mfn_valid() may return false for MMIO pages, for which it may
> still
> be legitimate to create mappings. IMO ...
> 
> > We could use a placeholder for the RAM end (for example, a very
> > high
> > value like -1UL) and then add __mfn_valid() within pt_update().
> > However, I'm not sure if this approach aligns with what you
> > consider by
> > proper separation between INVALID_MFN and "invalid MFN."
> 
> ... throughout the code here you mean INVALID_MFN and never "invalid
> MFN".
IIC INVALID_MFN should mean that mfn exist ( correspond to some usable
memory range of memory map ) but hasn't been mapped yet. Then for me
what I have in the comment seems correct to me:
```
   if `mfn` isn't equal to INVALID_MFN ( so it is valid/exist in terms
   that there is real memory range in memory map to which this mfn
   correspond ) and flags PTE_VALID bit set ...
```


> Populating page tables is lower a layer than where you want to be
> concerned with that distinction; the callers of these low level
> functions
> will need to make the distinction where necessary.
Then the question now is just in a proper wording of the pt_update()
arguments values?

~ Oleksii