[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v7 7/8] xen/riscv: page table handling
On Fri, 2024-09-27 at 11:15 +0200, Jan Beulich wrote: > On 27.09.2024 10:49, oleksii.kurochko@xxxxxxxxx wrote: > > On Wed, 2024-09-25 at 16:58 +0200, Jan Beulich wrote: > > > On 25.09.2024 16:50, oleksii.kurochko@xxxxxxxxx wrote: > > > > On Wed, 2024-09-25 at 16:22 +0200, Jan Beulich wrote: > > > > > On 25.09.2024 12:07, oleksii.kurochko@xxxxxxxxx wrote: > > > > > > On Tue, 2024-09-24 at 15:31 +0200, Jan Beulich wrote: > > > > > > > On 24.09.2024 13:30, oleksii.kurochko@xxxxxxxxx wrote: > > > > > > > > On Tue, 2024-09-24 at 12:49 +0200, Jan Beulich wrote: > > > > > > > > > On 13.09.2024 17:57, Oleksii Kurochko wrote: > > > > > > > > > > +static int pt_next_level(bool alloc_tbl, pte_t > > > > > > > > > > **table, > > > > > > > > > > unsigned > > > > > > > > > > int offset) > > > > > > > > > > +{ > > > > > > > > > > + pte_t *entry; > > > > > > > > > > + mfn_t mfn; > > > > > > > > > > + > > > > > > > > > > + entry = *table + offset; > > > > > > > > > > + > > > > > > > > > > + if ( !pte_is_valid(*entry) ) > > > > > > > > > > + { > > > > > > > > > > + if ( !alloc_tbl ) > > > > > > > > > > + return XEN_TABLE_MAP_FAILED; > > > > > > > > > > + > > > > > > > > > > + if ( create_table(entry) ) > > > > > > > > > > + return XEN_TABLE_MAP_FAILED; > > > > > > > > > > > > > > > > > > You're still losing the -ENOMEM here. > > > > > > > > Agree, I will save the return value of create_table and > > > > > > > > return > > > > > > > > it. > > > > > > > > > > > > > > That won't work very well, will it? > > > > > > I think it will work, just will be needed another one check > > > > > > in > > > > > > pt_update_entry() where pt_next_level() is called: > > > > > > if ( (rc == XEN_TABLE_MAP_FAILED) || (rc == - > > > > > > ENOMEM) ) > > > > > > ... > > > > > > > > > > Yet that's precisely why I said "won't work very well": > > > > > You're > > > > > now > > > > > having > > > > > rc in two entirely distinct number spaces (XEN_TABLE_MAP_* > > > > > and - > > > > > E*). > > > > > That's imo just calling for trouble down the road. Unless you > > > > > emphasized > > > > > this aspect pretty well in a comment. > > > > > > > > > > > > Imo you need a new XEN_TABLE_MAP_NOMEM. > > > > > > > (And then XEN_TABLE_MAP_FAILED may want renaming to e.g. > > > > > > > XEN_TABLE_MAP_NONE). > > > > > > I am still curious if we really need this separation. If to > > > > > > in > > > > > > this > > > > > > way > > > > > > then it should be updated the check in pt_update_entry(): > > > > > > --- a/xen/arch/riscv/pt.c > > > > > > +++ b/xen/arch/riscv/pt.c > > > > > > @@ -165,10 +165,10 @@ static int pt_next_level(bool > > > > > > alloc_tbl, > > > > > > pte_t > > > > > > **table, unsigned int offset) > > > > > > if ( !pte_is_valid(*entry) ) > > > > > > { > > > > > > if ( !alloc_tbl ) > > > > > > - return XEN_TABLE_MAP_FAILED; > > > > > > + return XEN_TABLE_MAP_NONE; > > > > > > > > > > > > if ( create_table(entry) ) > > > > > > - return XEN_TABLE_MAP_FAILED; > > > > > > + return XEN_TABLE_MAP_NOMEM; > > > > > > } > > > > > > > > > > > > if ( pte_is_mapping(*entry) ) > > > > > > @@ -209,7 +209,7 @@ static int pt_update_entry(mfn_t > > > > > > root, > > > > > > unsigned > > > > > > long virt, > > > > > > for ( ; level > target; level-- ) > > > > > > { > > > > > > rc = pt_next_level(alloc_tbl, &table, > > > > > > offsets[level]); > > > > > > - if ( rc == XEN_TABLE_MAP_FAILED ) > > > > > > + if ( (rc == XEN_TABLE_MAP_NONE) && (rc == > > > > > > XEN_TABLE_MAP_NOMEM) > > > > > > ) > > > > > > { > > > > > > rc = 0; > > > > > > But the handling of XEN_TABLE_MAP_NONE and > > > > > > XEN_TABLE_MAP_NOMEM > > > > > > seems to > > > > > > me should be left the same as this one part of the code > > > > > > actually > > > > > > catching the case when create_table() returns -ENOMEM: > > > > > > pt_next_level() > > > > > > { > > > > > > ... > > > > > > if ( flags & (PTE_VALID | PTE_POPULATE) ) > > > > > > { > > > > > > dprintk(XENLOG_ERR, > > > > > > "%s: Unable to map level %u\n", > > > > > > __func__, > > > > > > level); > > > > > > rc = -ENOMEM; > > > > > > } > > > > > > > > > > Except that you want to avoid "inventing" an error code when > > > > > you > > > > > were > > > > > handed one. Just consider what happens to this code if > > > > > another - > > > > > E... > > > > > could also come back from the helper. > > > > I think we can drop the usage of -ENOMEM in the helper > > > > create_table() > > > > by returning XEN_TABLE_MAP_FAILED in case of failure, with a > > > > redefinition of XEN_TABLE_MAP_FAILED = 1, XEN_TABLE_SUPER_PAGE > > > > = 2, > > > > and > > > > XEN_TABLE_NORMAL = 3, as value 0 is used to indicate that > > > > everything is > > > > okay. > > > > > > > > We can leave the pt_update() code as it is now: > > > > ... > > > > if ( flags & (PTE_VALID | PTE_POPULATE) ) > > > > { > > > > dprintk(XENLOG_ERR, > > > > "%s: Unable to map level %u\n", __func__, > > > > level); > > > > rc = -ENOMEM; > > > > } > > > > ... > > > > > > > > Because for the end user, it's better to receive the error code > > > > from > > > > xen/errno.h rather than a custom error code introduced nearby > > > > the > > > > helper. > > > > > > > > Does it make sense? > > > > > > While I think I see where you're coming from, I still don't > > > agree. > > > And > > > I never suggested to bubble up some custom error indication. Up > > > the > > > call > > > chain it wants to be -ENOMEM, sure. Yet keying its generation to > > > flags & (PTE_VALID | PTE_POPULATE) is both less obvious and more > > > fragile > > > (towards future code changes) than keying it to rc == > > > XEN_TABLE_MAP_NOMEM. > > I am not sure that (rc == XEN_TABLE_MAP_NOMEM) is equal to (flags & > > (PTE_VALID | PTE_POPULATE) as XEN_TABLE_MAP_NOMEM miss the case > > (flags > > & PTE_VALID) == 0 ( removing a mapping case ) and for which should > > be > > returned 0 but not -ENOMEM. > > The intention is quite clear: Return -ENOMEM if and only if an > allocation > failed. Hence why I think the XEN_TABLE_MAP_NOMEM approach is > preferable. Just to be sure. Do you mean the following: diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c index d7eb207ddc..6cd2e595b6 100644 --- a/xen/arch/riscv/pt.c +++ b/xen/arch/riscv/pt.c @@ -209,24 +209,15 @@ static int pt_update_entry(mfn_t root, unsigned long virt, for ( ; level > target; level-- ) { rc = pt_next_level(alloc_tbl, &table, offsets[level]); - if ( rc == XEN_TABLE_MAP_FAILED ) + if ( rc == XEN_TABLE_MAP_NOMEM ) { - rc = 0; - - /* - * We are here because pt_next_level has failed to map - * the intermediate page table (e.g the table does not exist - * and the pt couldn't be allocated). It is a valid case when - * removing a mapping as it may not exist in the page table. - * In this case, just ignore it. - */ - if ( flags & (PTE_VALID | PTE_POPULATE) ) - { - dprintk(XENLOG_ERR, - "%s: Unable to map level %u\n", __func__, level); - rc = -ENOMEM; - } + rc = -ENOMEM; + goto out; + } + if ( rc == XEN_TABLE_MAP_NONE ) + { + rc = 0; goto out; } ~ Oleksii
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |